-
Notifications
You must be signed in to change notification settings - Fork 357
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: [M3-6724]: Linode Config & Interface endpoints, validation, and React Query queries #9418
feat: [M3-6724]: Linode Config & Interface endpoints, validation, and React Query queries #9418
Conversation
…s -- tweaks forthcoming
)}/configs/${encodeURIComponent(configId)}/interfaces` | ||
), | ||
setMethod('GET') | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this ever going to get sorted and/or paginated? If so you need params and x-filter. I think x-filter should be the minimum if we're going to present this data in a table which I assume we wil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a non-paginated finite list returned in devnum order. I asked the API team about params and filters on Wednesday morning; there are no plans to add any at the moment, and based on our UI wireframes, we don't have a real need for them as there won't be a new table for this specific data.
packages/manager/src/App.tsx
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Temporary code to facilitate testing. Will be removed prior to merging.
const IP_EITHER_BOTH_NOT_NEITHER = | ||
'A subnet must have either IPv4 or IPv6, or both, but not neither.'; | ||
|
||
export const vpcsValidateIP = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Across the Linodes, Firewalls, and Databases areas of the codebase, we already have several IP-related validation functions, but they all have slightly different quirks. I think a more thorough-going look at all of those to see if they can be consolidated should be done at some point.
In the meantime I'm trying to avoid adding several more IP-related validation functions. There's a good argument this function is over-engineered, but it captures what we need for these VPC-related validations without significantly growing the number of IP-related validation functions in the codebase. That being said, if there's consensus around breaking this function up, we can give that concern more weight than the concern re: the number of IP-related validation functions.
[ | ||
['ipv6', 'ipv4'], | ||
['ipv4', 'ipv6'], | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The array prevents the cyclical dependency error.
primary?: boolean; | ||
subnet?: number | null; | ||
ipv4?: ConfigInterfaceIPv4; | ||
ipv6?: ConfigInterfaceIPv6; | ||
ip_ranges?: string[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we create a new LinodeConfigInterface that extends Interface instead of having all these optional props?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on how Interface
is used across the codebase, when going through the files this seems clearer to me
value?: string | null, | ||
shouldHaveIPMask?: boolean, | ||
mustBeIPMask?: boolean | ||
): boolean => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: This may be a personal preference, but updating the method to accept an object for parameters will not only make it more flexible/extensible, but also help others understand what these arguments are doing. The invocation vpcsValidateIP(value, true, false)
kinda speaks for itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DevDW I'm going to merge this so work can continue, but comments remain!
… React Query queries (#9418) Co-authored-by: Dajahi Wiley <dwiley@linode.com>
… React Query queries (#9418) Co-authored-by: Dajahi Wiley <dwiley@linode.com>
… React Query queries (#9418) Co-authored-by: Dajahi Wiley <dwiley@linode.com>
Description 📝
Add and modify existing Linode Config & Interface endpoints, validation, and RQ queries.
Major Changes 🔄
linodes.schema.ts
, modification tolinodeInterfaceSchema
vpcs.schema.ts
How to test 🧪
As in the previous couple of PRs, there is some temporary code in
App.tsx
to work with a RQ query.This PR significantly modifies the long-existing
linodeInterfaceSchema
, as well as newer VPC-related schemas added in previous PRs.If you'd like to test those schemas thoroughly against the API spec, the best way would be by creating a new local project and pulling in this branch's api-v4 and validation packages as dependencies. Your
package.json
file in your project would look something like:From there you'd have a file like
file.js
, import the schemas you want to test, and create mock payloads. As an example:And then run a node command like
node .src/file.js
to execute the code.Beyond this, please confirm that you can still create Linodes in Cloud -- there were modifications made to
linodeInterfaceSchema
so that impacts the Linode creation payload.